-
Notifications
You must be signed in to change notification settings - Fork 212
Fix issue #212 #272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue #212 #272
Conversation
@@ -22,6 +22,29 @@ import './style.scss'; | |||
const CHALLENGE_PLACEHOLDER_COUNT = 8; | |||
|
|||
export default function ChallengeListing(props) { | |||
function CheckDateTime(str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twicoder Ha! Love your efforts here, and most probably, this code behaves properly (have not tested), but I believe, the best way to do this check is actually just using momentjs
, doing moment(date).isValid()
: https://momentjs.com/docs/#/parsing/is-valid/
if (d.getSeconds() !== r[6]) return false; | ||
return true; | ||
} | ||
const filterState = props.filterState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twicoder And, although it probably will do the trick, the best way to make the check and avoid using the date in filter, if it is invalid, is inside reducer (the filter query params are picked up from URL during server-side rendering, thus checking it there will guarantee that invalid dates won't be used anywhere): https://github.com/topcoder-platform/community-app/blob/develop/src/shared/reducers/challenge-listing/index.js#L272
Also, once at the client-side, we should remove invalid dates from query. This can be done at this point. To update query params use this utility function: https://github.com/topcoder-platform/community-app/blob/develop/src/shared/utils/url.js#L20
@birdofpreyru I have changed the code as suggested. |
@@ -270,6 +271,14 @@ export function factory(req) { | |||
|
|||
if (req) { | |||
state.filter = req.query.filter; | |||
if (!!state.filter && !!state.filter.startDate | |||
&& moment(state.filter.startDate).isValid() === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... It is enough to write
if (state.filter && state.filter.stateDate && moment(state.filter.startDate).isValid()) {
not sure, why you don't do it this way? Please fix.
src/shared/utils/url.js
Outdated
} else if (!!query.filter && !!query.filter.endDate | ||
&& moment(query.filter.endDate).isValid() === false) { | ||
delete query.filter.endDate; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you was not supposed to modify updateQuery(..)
what I was talking about was to make such checks in src/shared/routes/Topcoder/ChallengeListing.jsx
and if you see we need to remove these query params, then you do it by calling updateQuery({ startDate: undefined, endDate: undefined })
. The updateQuery(..)
method is just an auxiliary function for setting / updating / deleting query params from URL. That's it.
@birdofpreyru Changed as suggestion, thanks! |
The fix will check the startDate and endDate, if date is invalid, then ignore them in the search logic.